Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

here are my thoughts on this. thanks!

• Folder name: web/ (proposed), dashboard/, apps/web/?

I would go for apps/web so it will be compliant with the monorepo.

• Workspace: should we plan a follow-up PR introducing a pnpm-workspace.yaml and an apps/ + packages/ layout?

Doing it now would be a single big move; doing it later as its own PR keeps this one focused on the import.
Yes in another PR

• Shared parser: extracting a parser package from both src/extension/parsers/ and web/src/lib/bmad/ makes sense long-term, but it's a non-trivial refactor that touches both surfaces. Worth a dedicated discussion before any code change.

Review and merge ownership going forward on this repo.
In my opinion, it should be done right after the monorepo in a shared package

PR Review: #5

Title: feat: import MyBMAD web dashboard alongside VS Code extension
Author: @DevHDI
Branch: feature/import-mybmad-web-app → main
Review layers: Adversarial + Edge Case Hunter

Findings

1. Static imports hoisted above ALLOW_REGISTRATION env assignment in create-admin script [likely]

Severity: 🔴 Critical
Source: [Both]

web/scripts/create-admin.ts — The script sets process.env.ALLOW_REGISTRATION = "true" on line 9, then uses static import statements for prisma and auth. In Node.js ESM (and TypeScript compiled to ESM), static imports are hoisted and evaluated before any module-level code executes. By the time the assignment runs, the auth module has already initialized with ALLOW_REGISTRATION unset — so the registration gate remains closed and the sign-up call will fail silently in environments where registration is disabled.

Suggested fix:

process.env.ALLOW_REGISTRATION = "true";
// Use dynamic imports so the env var is set before module initialization
const { prisma } = await import("../src/lib/db/client");
const { auth } = await import("../src/lib/auth/auth");

2. Sign-up 422 error reveals whether an email is already registered [likely]

Severity: 🔴 Critical
Source: [Adversarial]

web/src/app/login/login-form.tsx — The error handler returns a distinct user-visible message for HTTP 422 ("Un compte existe déjà avec cet email.") versus any other sign-up failure. This allows an unauthenticated attacker to enumerate registered email addresses by iterating a list and observing response differences. No rate limit is visible in this code path.

Suggested fix: Use a single, ambiguous message for all non-403 sign-up failures — e.g., "Erreur lors de l'inscription." — and do not distinguish "email already taken" from other errors in the client-facing response.

3. Docker CMD starts the application unconditionally after a failed database migration [likely]

Severity: 🔴 Critical
Source: [Both]

web/Dockerfile line 164 — The shell expression prisma migrate deploy || echo 'Warning: migration skipped'; node server.js is parsed as (prisma migrate deploy || echo '...') ; node server.js. The semicolon binds at the statement level, so node server.js runs regardless of whether migrations succeeded. If the database schema is out of sync, the application starts against a mismatched schema, risking runtime errors or data corruption on first query.

Suggested fix:

CMD ["sh", "-c", "prisma migrate deploy && node server.js"]
# Or, to surface migration failures explicitly:
# CMD ["sh", "-c", "prisma migrate deploy || { echo 'Migration failed, aborting'; exit 1; }; node server.js"]

4. No Content-Security-Policy header [likely]

Severity: 🔴 Critical
Source: [Adversarial]

web/next.config.ts — The global headers configuration sets X-Frame-Options, X-Content-Type-Options, and Referrer-Policy, but omits Content-Security-Policy. For an authenticated application that handles session tokens and renders content from external sources (GitHub avatars, user-controlled repo content), the absence of CSP meaningfully broadens the impact of any XSS vector. A baseline policy (default-src 'self', locked-down exceptions for image sources, etc.) should accompany the other security headers.

5. Incomplete path traversal validation — absolute paths and URL-encoded sequences not blocked [likely]

Severity: 🔴 Critical
Source: [Both]

web/src/actions/repo-actions.ts — fetchFileContentSchema — The schema refinement blocks .. sequences but does not block absolute paths (/etc/passwd) or URL-percent-encoded traversals (%2e%2e/secret). Relying on LocalProvider to enforce boundaries downstream is not a substitute for validation at the schema layer; a future change to the provider could silently remove that protection.

Suggested fix:

path: z.string().min(1).max(1024).trim()
  .refine((p) => {
    const decoded = decodeURIComponent(p);
    return (
      !decoded.startsWith("/") &&
      !decoded.includes("\0") &&
      !/(?:^|[\\/])\.\.(?:[\\/]|$)/.test(decoded)
    );
  }, { message: "Invalid path" }),

6. Cache tag invalidated before GitHub fetch succeeds in refreshGitHubRepo [likely]

Severity: 🔴 Critical
Source: [Edge Case]

web/src/actions/repo-actions.ts → refreshGitHubRepo — revalidateTag(...) is called unconditionally before the octokit.rest.git.getTree fetch. If that fetch throws (network error, rate limit, 404), the cache is already invalidated: the next render cycle fetches stale or empty data with no recovery path until a future successful refresh.

Suggested fix: Move revalidateTag to after the fetch and DB write succeed:

const { data: tree } = await octokit.rest.git.getTree({ ... });
await prisma.repo.update({ ... });
revalidateTag(repoTag(input.owner, input.name)); // only on success
return { success: true, ... };

7. listRepoBranches makes unscoped GitHub API call when repo does not belong to the requesting user [likely]

Severity: 🔴 Critical
Source: [Edge Case]

web/src/actions/repo-actions.ts → listRepoBranches — After the local-repo guard, if repoConfig is null (repo does not exist in the dashboard or belongs to a different user), the function falls through and issues a GitHub listBranches call using owner/name values taken directly from user input. An authenticated user can supply arbitrary owner/name values to enumerate branches of repositories they do not own within the dashboard.

Suggested fix:

if (!repoConfig) {
  return { success: false, error: "Repository not found", code: "NOT_FOUND" };
}
if (repoConfig.sourceType === "local") {
  return { success: false, error: "Branch management is not available for local projects", code: "NOT_APPLICABLE" };
}
// ... proceed with GitHub call

8. Malformed JSON body causes unhandled exception (500) in revalidate route [likely]

Severity: 🔴 Critical
Source: [Edge Case]

web/src/app/api/revalidate/route.ts — await request.json() is not wrapped in a try/catch. A request with a malformed body (empty body, wrong Content-Type, truncated payload) throws a SyntaxError that is not caught, resulting in a 500 instead of a 400. This also bypasses any error-monitoring middleware that expects structured error responses.

Suggested fix:

let body: unknown;
try {
  body = await request.json();
} catch {
  return NextResponse.json({ error: "Invalid JSON body" }, { status: 400 });
}
const parsed = bodySchema.safeParse(body);

9. revalidateTag called with a spurious second argument [likely]

Severity: 🟡 Moderate
Source: [Adversarial]

web/src/actions/repo-actions.ts — Multiple call sites pass "default" as a second argument to revalidateTag (e.g., revalidateTag(repoTag(input.owner, input.name), "default")). Next.js revalidateTag accepts exactly one argument; the second is silently ignored at runtime. If the intent is to also invalidate the "default" tag, a separate revalidateTag("default") call is needed. As written, the call is misleading and may mask a caching bug where "default" is never actually invalidated.

10. In-memory rate limiter does not survive restarts or horizontal scaling [likely]

Severity: 🟡 Moderate
Source: [Adversarial]

web/src/actions/repo-actions.ts — checkRateLimit is backed by process memory. Every container restart, rolling deploy, crash loop, or new pod instance resets all rate limit state. In a containerized environment (which this Dockerfile implies), this limit is trivially bypassed. This is a known MVP tradeoff, but it is not documented anywhere in the codebase, which means the next developer will not know it is intentional.

Suggested fix: At minimum, add a comment noting the limitation and the intent to replace with a persistent store (e.g., Redis) before production scale-out. At maximum, use a shared cache for rate limit state.

11. .env.example ships with a realistic-looking dev database password [likely]

Severity: 🟡 Moderate
Source: [Adversarial]

web/.env.example line 19 — DATABASE_URL contains bmad_dev_password — a real-looking credential. Example files with plausible passwords have a consistent track record of being copied verbatim into production. The BETTER_AUTH_SECRET entry correctly leaves the value empty and instructs the developer to generate one. The same pattern should apply to the database password: use REPLACE_ME, <your-db-password>, or any clearly invalid placeholder.

12. role field is an unconstrained String — no database-level enforcement of valid values [likely]

Severity: 🟡 Moderate
Source: [Adversarial]

web/prisma/schema.prisma line 215 — role String @default("user") allows any string value at the database level. A direct DB write, a future missed validation branch, or a scripting error could insert values like "superadmin", "", or any arbitrary string. Switching to a Prisma enum enforces valid values at both the ORM and database (PostgreSQL CHECK constraint) levels and eliminates this class of privilege-escalation risk.

Suggested fix:

enum UserRole {
  user
  admin
}

model User {
  role  UserRole  @default(user)
  ...
}

13. Revalidate endpoint has no rate limit of its own [likely]

Severity: 🟡 Moderate
Source: [Adversarial]

web/src/app/api/revalidate/route.ts — Authentication relies on a static shared secret. If the secret is obtained or brute-forced, an attacker can issue unlimited revalidateTag calls, forcing continuous cache invalidation and elevating upstream load. A simple rate limit on this endpoint — even the same in-memory implementation used elsewhere — would meaningfully raise the bar.

14. detectBmadRepos accepts an unbounded input array [likely]

Severity: 🟡 Moderate
Source: [Edge Case]

web/src/actions/repo-actions.ts → detectBmadRepos — No maximum is enforced on repoIds.length. A caller with a valid session can pass thousands of repo IDs, causing ceil(N/30) sequential GraphQL requests against the GitHub API, exhausting the user's rate-limit quota and potentially blocking the event loop for multiple seconds.

Suggested fix:

const MAX_REPOS = 300;
if (repoIds.length > MAX_REPOS) {
  return { success: false, error: `Too many repos (max ${MAX_REPOS})`, code: "LIMIT_EXCEEDED" };
}

15. extendBmadDirs failure is silently swallowed in fetchFileContent [likely]

Severity: 🟡 Moderate
Source: [Edge Case]

web/src/actions/repo-actions.ts → fetchFileContent (local path, lines ~1271–1278) — When provider.extendBmadDirs(topSegment) throws, the catch block discards the error and continues to provider.getFileContent(requestedPath). The comment assumes getFileContent will independently enforce the path restriction, but this is a behavioral assumption rather than a guarantee. If the provider's allow-list was not extended and getFileContent does not independently validate, a file outside the intended bmad directories may be returned. The silent catch also removes any observability into failures.

Suggested fix:

try {
  provider.extendBmadDirs(topSegment);
} catch (err) {
  console.warn("extendBmadDirs failed for segment:", topSegment, err);
  return { success: false, error: sanitizeError(null, "CONFIG_ERROR"), code: "CONFIG_ERROR" };
}

16. detectBmadRepos batch failure is indistinguishable from "no BMAD repos found" [likely]

Severity: 🟢 Minor
Source: [Adversarial]

web/src/actions/repo-actions.ts → detectBmadRepos — A failed GraphQL batch silently sets all repos in that chunk to false with only a console.warn. From the user's perspective, their BMAD repositories silently disappear. Returning null or a sentinel for "unknown" instead of false for "confirmed negative" would allow the UI to distinguish an error state from a clean absence, and surface the failure to the user.

17. Hardcoded parsingErrorRate: 0 presents unimplemented metric as real data [likely]

Severity: 🟢 Minor
Source: [Edge Case]

web/src/actions/admin-actions.ts → getUsageMetrics line ~560 — The metric is permanently 0 with no indication in the UI that tracking is not yet implemented. Admins may interpret 0% as "no parsing errors" when the data simply does not exist. The value should be null and the UI should render "N/A — not yet tracked" to avoid presenting a placeholder as an operational signal.

18. Redundant admin authentication at RSC level and in downstream server actions [likely]

Severity: 🟢 Minor
Source: [Adversarial]

web/src/app/(dashboard)/admin/page.tsx — The page performs getAuthenticatedSession() + role check and redirects before the server actions are ever invoked in this flow. Each action (getUsers, getUsageMetrics) then calls requireAdmin() again internally. In this call path the second check is dead code. While defense-in-depth at the action layer is generally good practice, the dual pattern without documentation creates ambiguity about where the authoritative auth boundary lives and may mislead future developers who call these actions from other callsites.

Summary

Critical: 8 | Moderate: 7 | Minor: 3
Sources: 5 adversarial | 5 edge case | 6 both

Note: No binary files were detected in this PR. The bulk of the 216 changed files are the full import of the web/ Next.js application. This review focused on authentication, authorization, server actions, database schema, and infrastructure configuration — the highest-risk surface areas for a new application import.

Review generated by Raven's Verdict. LLM-produced analysis — findings may be incorrect or lack context. Verify before acting.

Thanks for the detailed review.

Since this PR is intended as a verbatim import from DevHDI/my-bmad, I’m going to address the confirmed findings upstream first, then re-import the fixed upstream HEAD into this branch. That keeps the import auditable and avoids creating two divergent versions of the web app.

I’ll keep this PR open as draft while doing that. I’m also triaging the Raven findings before changing code: a few look like hardening or false positives rather than critical bugs, so I’ll document those explicitly in the PR description when the re-import is pushed.

Re-import complete — all confirmed Raven's Verdict findings addressed upstream

Force-pushed the branch to point at DevHDI/my-bmad@575f6f5, which is the post-merge state of four thematic fix PRs landed upstream this week:

• DevHDI/my-bmad#14 — security & input hardening (#7, #14, feat: introduce GitHub Copilot Chat adapter using official BMAD prompt artifacts #2)
• DevHDI/my-bmad#15 — revalidate route hardening (#6, #8, #13)
• DevHDI/my-bmad#16 — docker fail-fast & env hygiene (Initial extension contribution #3, #11, #10)
• DevHDI/my-bmad#17 — admin metrics & error logging (#17, #16, #15)

The PR description above has been rewritten with a full mapping table: each Raven finding → the upstream PR that fixed it (or the issue tracking it for follow-up, or the documented reason why it was a false positive — #1, #5, #9, #18 — with file/line evidence).

Two findings were intentionally deferred to dedicated follow-ups because they need design/migration work that doesn't belong in this import PR: #12 baseline CSP and #13 role enum migration.

This branch remains a verbatim git archive HEAD import from the upstream — just from a fixed HEAD instead of the pre-fix one. The single commit at the tip is still a clean re-import, no in-tree patches.

Happy to discuss any specific finding (especially the four marked false-positive) before this lands.

This pull request is abnormally large and would use a significant amount of tokens to review. If you still wish to review it, comment "augment review" and we will review it.